-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add missing test for ones and zeros #34948
Conversation
and add more tests for zeros and ones
https://docs.julialang.org/en/v1/base/arrays/#Base.ones You should show the current problem and the benefits of this PR. |
My only weakly defense is that abstract type should be treated differently given that it's the same case for It's likely to hit a performance issue there (although only once) julia> X1 = ones(AbstractFloat, 1000, 1000); # Array{AbstractFloat, 2}
julia> X2 = ones(Float64, 1000, 1000);
julia> Y = randn(Float64, 1000, 1000);
julia> @benchmark X1 .* Y
BenchmarkTools.Trial:
memory estimate: 22.89 MiB
allocs estimate: 1000011
--------------
minimum time: 47.891 ms (0.00% GC)
median time: 50.460 ms (0.00% GC)
mean time: 50.898 ms (1.31% GC)
maximum time: 58.578 ms (2.76% GC)
--------------
samples: 99
evals/sample: 1
julia> @benchmark X2 .* Y
BenchmarkTools.Trial:
memory estimate: 7.63 MiB
allocs estimate: 4
--------------
minimum time: 1.157 ms (0.00% GC)
median time: 1.488 ms (0.00% GC)
mean time: 2.728 ms (45.41% GC)
maximum time: 22.673 ms (92.12% GC)
--------------
samples: 1830
evals/sample: 1 |
For example, if you need an array of |
When julia> struct Foo end
julia> Base.one(::Type{Foo}) = 1
julia> Base.oneunit(::Type{Foo}) = Foo()
julia> ones(Foo, 3, 3)
ERROR: MethodError: Cannot `convert` an object of type Int64 to an object of type Foo
Closest candidates are:
convert(::Type{T}, ::T) where T at essentials.jl:168
Stacktrace:
[1] fill!(::Array{Foo,2}, ::Int64) at ./array.jl:309
[2] ones(::Type{Foo}, ::Tuple{Int64,Int64}) at ./array.jl:462
[3] ones(::Type{Foo}, ::Int64, ::Int64) at ./array.jl:457
[4] top-level scope at REPL[4]:1
julia> function jc_ones(::Type{T}, dims...) where T
init = one(T)
a = Array{typeof(init)}(undef, dims)
fill!(a, init)
end
jc_ones (generic function with 1 method)
julia> jc_ones(Foo, 3, 3)
3×3 Array{Int64,2}:
1 1 1
1 1 1
1 1 1 Given the decision in #24444, having Base throw an error is probably useful, as it encourages people to specify what they actually want by calling Note, however, that packages like Unitful (incorrectly, IMO) specialize |
Close this as the proposing change doesn't have much practical usage. |
I'm actually a little surprised that
eltype(ones(T)) === typeof(one(T))
not hold true for abstract types (e.g.,AbstractFloat
) given thatone(T)
is allowed, so here's one small patch that fixes this.However, it's also a conflict that
eltype(ones(T)) != T
... so feedbacks are needed.cc: @kimikage
Edit:
Actually I'm not convinced about this change at all, with two 👎 here I think it's sufficient to just reverted the changes and simply adds some test cases for zeros and ones.